Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sparkpost tracking config option #5

Merged
merged 8 commits into from
Mar 6, 2025
Merged

Sparkpost tracking config option #5

merged 8 commits into from
Mar 6, 2025

Conversation

escopecz
Copy link

@escopecz escopecz commented Mar 5, 2025

Motivation
The Sparkpost email open and click tracking was always disabled. This PR keeps it disabled but allows users to enable it via a configuration option sparkpost_tracking_enabled

Testing steps

  1. Send an email with some link via Sparkpost without any change in configuraiton. You should see by hovering over the link in the email that the URL goes to Mautic first. If you click the link Mautic redirects you to the original location. You should see the click event in Mautic but not in Sparkpost.
  2. Enable the Sparkpost tracking (See the changes in the readme)
  3. Send an email with some link via Sparkpost. You should see by hovering over the link in the email that the URL goes to Sparkpost first. If you click the link then Sparkpost redirects you to Mautic and Mautic redirects you to the original location. You should see the click event in Mautic as well as in Sparkpost.

@escopecz escopecz changed the title Tracking config Sparkpost tracking config option Mar 5, 2025
@escopecz escopecz requested a review from Copilot March 5, 2025 14:49

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR introduces a configuration option to enable Sparkpost tracking within the Mautic Sparkpost Plugin, along with updated installation and configuration instructions in the README.

  • Added a "Sparkpost tracking" section describing how to enable tracking via the configuration option in config/local.php.
  • Revised the README to include detailed installation methods and configuration examples.

Reviewed Changes

File Description
README.md Added installation options and documentation explaining how to enable Sparkpost tracking

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

README.md:56

  • [nitpick] Consider rephrasing the sentence for clarity. For example: 'Sparkpost tracking is disabled by default to prevent duplicate tracking of email opens and clicks (once by Sparkpost and once by Mautic), which could lead to unexpected behavior.'
The Sparkpost tracking is disabled by default as then the email open and clicks would be tracked twice. Once by Sparkpost, second time by Mautic. This can create some unexpected behavior.
@escopecz escopecz requested review from fedys and nileshlohar March 5, 2025 14:49
Copy link

@nileshlohar nileshlohar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM

Copy link

@fedys fedys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @escopecz! Please see my code comments.

@@ -127,7 +140,7 @@ private function assertSparkpostTestRequestBody(string $body): void
Assert::assertSame('Hi! This is a test email from Mautic. Testing...testing...1...2...3!', $bodyArray['content']['text']);
}

private function assertSparkpostRequestBody(string $body): void
private function assertSparkpostRequestBody(string $body, bool $expectedTrackingConfig): void
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter $expectedTrackingConfig is not used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! It turns out the assertions were muted by a code that was catching generic Exception. There were already several assertions failing but the tests were green. Fixed it.

Comment on lines 26 to 27
private TranslatorInterface&MockObject $translatorMock;
private CoreParametersHelper&MockObject $coreParametersHelper;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot use PHP's intersection types until we bump minimum PHP version to 8.1.
It is 8.0 now.

"php": ">=8.0.0",

@escopecz escopecz requested review from fedys and Copilot March 6, 2025 14:03

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR introduces a configuration option to enable Sparkpost tracking in the Mautic Sparkpost Plugin, allowing users to control whether click and open events are tracked by Sparkpost in addition to Mautic. Key changes include:

  • Adding a "Sparkpost tracking" section in the README with an explanation on the behavior when the option is enabled.
  • Detailing configuration instructions for enabling Sparkpost tracking via the Mautic configuration file.

Reviewed Changes

File Description
README.md Added installation instructions and a new section for Sparkpost tracking configuration

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Copy link

@fedys fedys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @escopecz! 👍

@fedys fedys self-requested a review March 6, 2025 14:58
@escopecz escopecz merged commit ebcb52e into main Mar 6, 2025
1 check passed
@escopecz escopecz deleted the tracking-config branch March 6, 2025 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants